Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add test for spherical_coslat_differential #123

Closed
wants to merge 11 commits into from

Conversation

rjnrohit
Copy link

@rjnrohit rjnrohit commented Apr 5, 2020

Description

  1. Previously, cross generate errors for some coordinate system i.e previous version can't able to identify bu::conversion_factor(plane_angle, meter for cross(cartesian point, spherical point) ) in calculation part of code.
  2. Now cross returns cross product in requested representation( It returns correct units for every representation).
  3. Updated unit_vector function in arithmetic.hpp to return a unit vector directed from one point to another point.
  4. Update tests to check the cross product of representation.
  5. Test added for spherical_coslat_differential representation, update io.hpp header to support differential motion object.
    6.Now, cross can also calculate cross product of differential motion objects too.

References

Tasklist

  • Add test case(s)
  • Ensure all CI builds pass
  • Review and approve

@rjnrohit
Copy link
Author

rjnrohit commented Apr 5, 2020

@lpranam please review this PR and provide feedback to me. #123

@rjnrohit rjnrohit changed the title update cross in arithmetic.hpp Bug fix in cross in arithmetic.hpp and add test for spherical_coslat_differential Apr 10, 2020
DimensionCount,
CoordinateSystem
>
get_point() const
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Function to return current differential of this object. By doing this we are able to calculate cross of differential objects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at #123 (comment)

Copy link
Member

@sarthak2007 sarthak2007 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should submit different PRs for different contexts. Like in this case, separate PRs should be there for unit_vector, magnitude, tests.
Also, you should push after squashing the commits.

auto cross
(
cartesian_differential<Args1...> const& representation1,
Representation2<Args2...> const& representation2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you have made this function to support differentials, you should name the arguments differential1 and differential2 instead of representation1 and representation2 to maintain uniformity in the library. And change this whole function accordingly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok. I'll do that.

DimensionCount,
CoordinateSystem
>
get_point() const
Copy link
Member

@sarthak2007 sarthak2007 Apr 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the need of this function? get_differential is already there in base_differential.hpp.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By doing this we can extend all arithmetic function to work for differentials as well. Otherwise, we may get a conflict of .get_point() in all arithmetic functions.

Copy link
Member

@sarthak2007 sarthak2007 Apr 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this will help in magnitude function. But functions like sum, mean, difference, dot use make_cartesian_representation() and hence can't be extended to work for differentials by defining get_point for differential.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll also help to calculate cross for two differential classes or one representation class and one differential class. In the code where we used
bg::transform(cartesian1.get_point(), tempPoint1); bg::transform(cartesian2.get_point(), tempPoint2);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sarthak2007 I'm closing this PR. I have created a separate PR #127 for the extension of arithmetic functions to support differential systems.

@rjnrohit rjnrohit requested a review from sarthak2007 April 13, 2020 15:11
@rjnrohit
Copy link
Author

You should submit different PRs for different contexts. Like in this case, separate PRs should be there for unit_vector, magnitude, tests.
Also, you should push after squashing the commits.

Ok. I'll create a new PR for every context.

@rjnrohit rjnrohit changed the title Bug fix in cross in arithmetic.hpp and add test for spherical_coslat_differential add test for spherical_coslat_differential Apr 13, 2020
@rjnrohit rjnrohit closed this Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants